Allow passing pathlib.PosixPath to the function read_from_text_file()#787
Allow passing pathlib.PosixPath to the function read_from_text_file()#787xingularity wants to merge 1 commit into
pathlib.PosixPath to the function read_from_text_file()#787Conversation
xingularity
force-pushed
the
hotfix/handle-posix-path-issue-786
branch
from
a8ab00b to
9930fd0
Compare
|
Sorry maybe I didn't fully understand the test cases. I wonder are the test cases be able to catch the errors that happens in #786? |
Yes, test case |
yungyuc
changed the title
pathlib.PosixPath to the function read_from_text_file()
|
I changed the subject to be more informative. |
@yungyuc and @tigercosmos The comment has been added, you can review now. |
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
- Use inline annotation for discussions in the PR, not code comments.
- Also check for error messages, not just the exception type.
| finally: | ||
| os.unlink(path) | ||
|
|
||
| # Theis test is for a bug reported in issue #786: |
There was a problem hiding this comment.
It seems that you misunderstood what we meant by inline annotations. In #787 (review), "inline annotations in the PR" is like the inline comment I am leaving here. It is not code remarks/comments.
Please remove the unnecessary code comments and turn it into a PR inline annotation.
There was a problem hiding this comment.
Nitpick: there is a typo Theis.
There was a problem hiding this comment.
| nd_arr = np.genfromtxt(io.StringIO(self.dlc_data), delimiter=',')[1:] | ||
| self.assertEqual(list(col_data), list(nd_arr[:, 1])) | ||
|
|
||
| def test_read_from_text_file_accepts_str_path(self): |
There was a problem hiding this comment.
This does not look like related to the fix. Please share with us why do you want to include it.
There was a problem hiding this comment.
The file path can be either a string or a os.PathLike instance. This test is added is to explicitly test if the DataFrame can accept str type text file path.
| def test_read_from_text_file_missing_raises_filenotfound(self): | ||
| tsdf = dataframe.DataFrame() | ||
| missing = pathlib.Path(tempfile.gettempdir()) / 'no_such_file.csv' | ||
| with self.assertRaises(FileNotFoundError): |
There was a problem hiding this comment.
Also check for error messages, not just the exception type.
xingularity
force-pushed
the
hotfix/handle-posix-path-issue-786
branch
2 times, most recently
from
35bac1d to
114f43d
Compare
xingularity
force-pushed
the
hotfix/handle-posix-path-issue-786
branch
from
114f43d to
0235bf6
Compare
1. Modify DataFrame code to accept os.PathLike csv paths. 2. Add tests to explicitly test if dataframe can accept paths from `pathlib` as well as a string 3. Enhance the exception raise to explicitly indicates the files are not found. 4. Add tests to test raised Exception type as well as the error message.
xingularity
force-pushed
the
hotfix/handle-posix-path-issue-786
branch
from
0235bf6 to
16c80aa
Compare
3 participants
Modified if statement in modmesh/track/dataframe. This is to handle file name as a PosixPath instance.
This is for issue #786.